⚙️ FEATURE-#21: Replace tool-card emoji with codicons, go-to-file, and auto-approved diffs - #22
Conversation
… and auto-approved diffs
.tool-icon/.tool-status/.tool-open-file sized themselves via plain single-class selectors, but codicon.css's own base rule (.codicon[class*='codicon-']) is an attribute selector — strictly higher specificity — and always won regardless of stylesheet load order, silently resetting font-size back to the library's 16px/1 default and leaving line-height unconstrained against the row. Scoped the sizing rules under .tool-card-header to out-specify it, pinned line-height:1 explicitly, and added overflow:hidden on the header as a hard clamp. Also dropped the leftover thinking-pulse opacity animation on .tool-status.pending — the pending status now renders via codicon-loading + codicon-modifier-spin, so the old animation was silently fighting it for the same CSS property.
codicon-modifier-spin's rotate animation applies to the whole
element box it's set on — but that same .tool-status element also
gets .auto-approved's `::after { content: " (auto)" }` text. Since a
transform rotates everything rendered in the element's box, the
"(auto)" label was spinning in a circle right along with the loading
glyph. Split the icon into its own inner .tool-status-icon span so
only the glyph carries the spin/codicon classes; .tool-status stays
a plain, non-rotating wrapper for the color state and the auto-
approved label.
FernandoCelmer
left a comment
There was a problem hiding this comment.
This PR cleanly replaces emoji tool-card icons with VS Code's native codicon font and adds a go-to-file action. The implementation is well-structured, tests are updated, and the CSP is correctly extended. A few items worth addressing before merge:
[Suggestion]
Problem — openFile resolves relative paths against currentWorkspaceFolder() with no sanitisation. A path like ../../.ssh/id_rsa is resolved and opened in the editor without any guard. Even though the webview is sandboxed, path traversal outside the workspace is undesirable and could expose sensitive files.
Failure scenario — A chat message triggers chat/autoApproved with a crafted tool whose args.path is ../../../home/user/.ssh/id_rsa; the user clicks the go-to-file button and VS Code opens the file.
Fix — After resolving the URI, verify it stays within the workspace root before calling showTextDocument:
private openFile(filePath: string): void {
const root = currentWorkspaceFolder();
const resolved = path.isAbsolute(filePath)
? filePath
: path.join(root, filePath);
const normalized = path.normalize(resolved);
if (!normalized.startsWith(path.normalize(root) + path.sep)) {
vscode.window.showErrorMessage(`Path outside workspace: ${filePath}`);
return;
}
const uri = vscode.Uri.file(normalized);
vscode.window.showTextDocument(uri).then(undefined, () => {
vscode.window.showErrorMessage(`Couldn't open ${filePath}`);
});
}[Suggestion]
Problem — The .tool-open-file:hover CSS rule is not scoped under .tool-card-header, unlike the sibling rule .tool-card-header .tool-open-file. This inconsistency means the hover style leaks globally: any future .tool-open-file element outside a .tool-card-header will pick up the colour change unexpectedly.
Failure scenario — A future refactor adds a .tool-open-file button in a different context (e.g. a result panel); its hover colour is silently affected by this rule.
Fix — Scope the hover rule to match its companion:
.tool-card-header .tool-open-file:hover {
color: var(--vscode-foreground);
}[Comment]
Problem — The openFile handler receives message.path directly from the webview and passes it to path.join / path.isAbsolute without any type narrowing beyond what TypeScript's union type provides at compile time. At runtime, a tampered message (e.g. via a browser DevTools injection into the webview) could supply a non-string value.
Failure scenario — message.path is null; path.isAbsolute(null) coerces to path.isAbsolute("null") returning false, then path.join(root, "null") opens a file named null in the workspace.
Fix — Add a runtime guard at the handler level, consistent with how other handlers coerce values (e.g. String(message.value ?? "")):
case "openFile":
if (typeof message.path === "string" && message.path.length > 0) {
this.openFile(message.path);
}
break;…age payload openFile() resolved a relative path against the workspace root with no check that the result stayed inside it — "../../.ssh/id_rsa" (or any tool-call arg pointing outside the workspace) would open in the editor unguarded. Now normalizes the resolved path and rejects it if it doesn't stay under the workspace root. Also guards the "openFile" message handler against a non-string/empty path, since it comes straight from the webview with no runtime type check beyond the compile-time union.
Its base rule (.tool-card-header .tool-open-file) is already scoped, but the :hover variant wasn't — any future .tool-open-file element outside a tool card would silently inherit this hover color.
|
Addressed all three review points: |
Description
Replace tool-card emoji with VS Code's native codicon font for a consistent look. Add a go-to-file button on file-affecting tool cards. Auto-approved file edits now render their diff preview in the chat via the new
previewfield onchat/autoApproved.Closes #21
Motivation and Context
Emoji rendering is inconsistent across platforms and themes. Codicons are the standard icon set for VS Code extensions and blend natively with the editor UI. The go-to-file action improves developer workflow by allowing direct navigation to touched files from the chat.
Types of changes
Checklist